Skip to content

Fix multiple nil identity columns for merge insert #1327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 12, 2025

Conversation

aidanharan
Copy link
Contributor

@aidanharan aidanharan commented May 7, 2025

Handle merge inserts that contain multiple nil identity columns. Eg:

Book.upsert_all [
  { id: nil, name: "New edition 1" },
  { id: nil, name: "New edition 2" },
]

Previously the SQL generated would have been:

MERGE INTO [books] WITH (updlock, holdlock) AS target
using (SELECT *
       FROM   (SELECT [id],
                      [name],
                      [created_at],
                      [updated_at],
                      [updated_on],
                      Row_number()
                        OVER (
                          partition BY [id]
                          ORDER BY [id] DESC ) AS rn_0
               FROM   (VALUES 
                             (Ident_current(N'[books]') + Ident_incr(N'[books]'),
                      N'New edition 2',
                      CURRENT_TIMESTAMP,
                      CURRENT_TIMESTAMP,
                      CURRENT_TIMESTAMP),
                              (Ident_current(N'[books]') + Ident_incr(N'[books]'),
                      N'New edition 1',
                      CURRENT_TIMESTAMP,
                      CURRENT_TIMESTAMP,
                      CURRENT_TIMESTAMP)) AS t1 ([id], [name], [created_at],
                      [updated_at],
                      [updated_on])) AS ranked_source
       WHERE  rn_0 = 1) AS source
ON ( target.[id] = source.[id] )
WHEN matched THEN
  UPDATE SET target.[updated_at] = CASE
                                     WHEN (( source.[name] = target.[name]
                                              OR ( source.[name] IS NULL
                                                   AND target.[name] IS NULL ) )
                                          ) THEN
                                     target.[updated_at]
                                     ELSE CURRENT_TIMESTAMP
                                   END,
             target.[updated_on] = CASE
                                     WHEN (( source.[name] = target.[name]
                                              OR ( source.[name] IS NULL
                                                   AND target.[name] IS NULL ) )
                                          ) THEN
                                     target.[updated_on]
                                     ELSE CURRENT_TIMESTAMP
                                   END,
             target.[name] = source.[name]
WHEN NOT matched BY target THEN
  INSERT ([id],
          [name],
          [created_at],
          [updated_at],
          [updated_on])
  VALUES (source.[id],
          source.[name],
          source.[created_at],
          source.[updated_at],
          source.[updated_on])
output inserted.[id]; 

When this is run only the book with title "New edition 1" is created in the table. The reason is that "New edition 2" is first inserted but then "New edition 1" is updated over "New edition 2" since it matches ON ( target.[id] = source.[id] ).

The fix changes the inserted ID columns to the following so that they don't match and both records get inserted:

  • Ident_current(N'[books]') + (Ident_incr(N'[books]') * 1)
  • Ident_current(N'[books]') + (Ident_incr(N'[books]') * 2)

This fixes the following tests which were added by rails/rails#54962

  • InsertAllTest#test_insert_all_implicitly_sets_primary_keys_when_nil
  • InsertAllTest#test_upsert_all_implicitly_sets_primary_keys_when_nil

@aidanharan aidanharan requested a review from andyundso May 8, 2025 13:02
@aidanharan aidanharan marked this pull request as ready for review May 8, 2025 13:02
@aidanharan aidanharan merged commit 1c5fb1f into main May 12, 2025
12 of 14 checks passed
@aidanharan aidanharan deleted the insert-all-default-primary-key branch May 12, 2025 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants